-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Pressable Group Updates #927
base: release/2.0.7
Are you sure you want to change the base?
Conversation
|
||
// Group Updated Message | ||
group_name_changed: '{{name}} changed the group name to "{{newValue}}".', | ||
group_member_joined: "{{name}} joined the conversation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@covrter This string has always felt that it's missing an explanation of how they got into the group. I think WhatsApp/Telegram/Signal all use variations of "{{inviter}} added {{joiner}}" for direct adds and "{{joiner}} joined via invite link". Should we do this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's valuable to include the inviter, because it communicates trust — do we have the data handy?
It does again raise the question of when the "join" happens — if I'm added by a non-consented contact "badboy", I would not expect to see "badboy added andrew" until I consent to join the convo. I might expect for "badboy invited andrew" to appear in the chat, and then when I consent "andrew joined" to appear. (This creates accountability for inviting people in, bc the group can see it.) Does that feel right?
Either way this is its own task :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have the information readily available for who added who (whom?)
But the other members of the group do not know if/when a different user "approves" (currently labeled as joins) the group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just riffing here, and this would be out of scope for now, but could the approval consent message be presented as a group update message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the consent popup? "Do you want to join this group?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It triggers a consent message, but that's on a different topic that other users in the group could not decrypt themselves (I cannot see your consent preferences)
The app can trigger some custom content type, but I think that's getting close to concerns with privacy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. What is the privacy concern with using a content type message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing consent of a group to the entire rest of the group.
If you "accept" a group, and then the app sends a message to tell everyone you have consented to the group might be a little weird
Unless we avoid showing the Group Updated messages and only show the Converse Group Consent message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably missing something, but that seems desirable. Group membership is already public to group members, and group consent creates a useful division between "invited" and "joined". We should check how other apps handle.
/cc @covrter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we would need to be mindful of how we are labelling these things
If Converse is relying on that, we need to run through all previous messages to see who's "consented" to the group, which will likely not be a complete list unless you are the creator of the group
I just want to ensure these UI/UX changes are P0 or maybe P1, since we’ll redo most of it later with the design system. Also, the tests will likely break after refactoring, making things harder to refactor. So my suggestion would be to not do any tests for now. |
@thierryskoda I'll stop with component tests, but we need to find a balance between new features and introducing the design system |
Added handling when pressing a display name in the group updated messages
Fixed tsconfig Added util to create text styles Updated Chat Group Updated message to match design system Added ParsedText component
6060c63
to
a64e62d
Compare
@thierryskoda Updated with the design system: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this loom before reading my comments https://www.loom.com/share/c4ebb553b56d48ad9fef405144f21fbc
import { ITextProps } from "@design-system/Text"; | ||
import { ITextStyleProps } from "@design-system/Text/Text.props"; | ||
import { ParseShape } from "react-native-parsed-text"; | ||
|
||
export interface IParsedTextProps extends ITextProps { | ||
parse: ParseShape[]; | ||
pressableStyle?: ITextStyleProps; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we shouldn't create a new file for props if they are small
profileStyle: { | ||
fontWeight: "bold", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used? 😮
I checked because we have a weight props for Text so we can just use this instead
} | ||
); | ||
|
||
export const ParsedText = memo(ParsedTextInner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we can put all inline like this:
export const ParsedText = memo(
forwardRef<RNParsedText, IParsedTextProps>((props, ref) => {
...
const pattern = useMemo( | ||
() => new RegExp(profileDisplay, "g"), | ||
[profileDisplay] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love having a space between things
...props.pressableStyle, | ||
}; | ||
}, [props]); | ||
const pressableStyles = getTextStyle(themed, childThemedProps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space please 🙏
const ParsedTextInner = forwardRef<RNParsedText, IParsedTextProps>( | ||
(props, ref) => { | ||
const { themed } = useAppTheme(); | ||
const styles = getTextStyle(themed, props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space please 🙏
const childThemedProps = useMemo(() => { | ||
return { | ||
...props, | ||
...props.pressableStyle, | ||
}; | ||
}, [props]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useMemo for something like this is overkill and even worst than not using it. Since we pass props in deps array. I would just do
const childThemedProps = { ...props, ...props.pressableStyle }
} from "./Text.styles"; | ||
|
||
export const getTextStyle = ( | ||
themed: ReturnType<typeof useAppTheme>["themed"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll use this type a lot. worth it exporting a IThemed
in useAppTheme so we can reuse that everywhere.
preset="subheading" | ||
size="xxs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having preset
and size
feels weird here. The size should be in the preset no?
import { ITextStyleProps } from "@design-system/Text/Text.props"; | ||
import { ParseShape } from "react-native-parsed-text"; | ||
|
||
export interface IParsedTextProps extends ITextProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're extending our text props but the component rendered is not our Text component? Maybe I'm missing something but I don't think it's a good pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the lib as ParsedTextProps
so maybe we should extend that instead? I see parse
is optional but we can make it required for us.
Added handling when pressing a display name in the group updated messages